Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Blender export flags for 3.6. #81194

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

rcorre
Copy link
Contributor

@rcorre rcorre commented Aug 31, 2023

Fixes #76338.

Blender 3.6 imports fail with:

TypeError: Converting py args to operator properties: : keyword "export_nla_strips" unrecognized

The export_nla_strips flag was removed and replaced with export_animation_mode.
In 3.6.0-3.6.21, this option does not exist at all and causes the failure above.
In 3.6.22, this option was re-added, but does nothing.
See https://projects.blender.org/blender/blender-addons/commit/96a73cb664bca687b7ea2e464c4d08f8082d5012.

We now need to check the blender version to determine what flags to use.
This adds an additional shell command before every import.
We might consider caching the version, but we'd have to invalidate the cache if the blender version or path changes.

As an aside, the "group animations" setting in Godot does the opposite of what I'd expect.
When group_tracks=true, each animation is exported individually.
When group_tracks=false, all animations are exported as a single track.
This seems backwards, but I've kept the 3.6 behavior consistent with 3.5.

From https://docs.blender.org/api/3.6/bpy.ops.export_scene.html:

ACTIONS Actions – Export actions (actives and on NLA tracks) as separate animations.
ACTIVE_ACTIONS Active actions merged – All the currently assigned actions become one glTF animation.

@rcorre rcorre requested a review from a team as a code owner August 31, 2023 12:50
@akien-mga
Copy link
Member

We now need to check the blender version to determine what flags to use.
This adds an additional shell command before every import.
We might consider caching the version, but we'd have to invalidate the cache if the blender version or path changes.

I think it's worth caching, only for a given Godot session (so in a member variable, not saved to disk).

filesystem/import/blender/blender3_path is already set to request a restart when changed, and if users change the Blender version without changing that path (e.g. relying on /usr/bin/blender and their distro upgrades), I think it's fine if we don't catch this for one session (the workaround would be to restart the editor after seeing that import don't work).

@akien-mga akien-mga added bug topic:import cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Aug 31, 2023
@akien-mga akien-mga added this to the 4.2 milestone Aug 31, 2023
@rcorre
Copy link
Contributor Author

rcorre commented Aug 31, 2023

I added caching and fixed the version checks. Thanks!

@rcorre
Copy link
Contributor Author

rcorre commented Sep 17, 2023

BTW, is anyone able to comment on this?

As an aside, the "group animations" setting in Godot does the opposite of what I'd expect.
When group_tracks=true, each animation is exported individually.
When group_tracks=false, all animations are exported as a single track.
This seems backwards, but I've kept the 3.6 behavior consistent with 3.5.

Is this the expected behavior of group_tracks?

@fire fire requested review from a team September 17, 2023 15:19
@fire
Copy link
Member

fire commented Sep 17, 2023

When group_tracks=true, each animation is exported individually.
When group_tracks=false, all animations are exported as a single track.

That sounds backwards, it's probably a bug and we need need to retire / rename the setting as a new variable with a transfer function.

@rcorre
Copy link
Contributor Author

rcorre commented Sep 24, 2023

That sounds backwards, it's probably a bug and we need need to retire / rename the setting as a new variable with a transfer function.

I'll do that in a separate PR to avoid complicating this one.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. We discussed in this thread what to do with group_tracks and there's a related pr on upwards compatibility that we're superseding.

Since this is a widely used feature we'll get better feedback on any edge cases with testing.


#if defined(MACOS_ENABLED)
if (!FileAccess::exists(path)) {
path = path.path_join("Blender");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit odd. I didn't want to delay the merge, but isn't the mac case-insensitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APFS is case sensitive by default, but can be configured as case-sensitive: https://support.apple.com/guide/disk-utility/file-system-formats-dsku19ed921c/mac

Copy link
Member

@akien-mga akien-mga Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work though?

This code tests p_path + "/blender", and then if it fails it tests p_path + "/blender/Blender". That seems a bit arbitrary and not just a matter of case sensitivity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that seems wrong. I can change it, but I don't have a mac to test on (again, this is moved code, not changed code)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it, as it did seem most likely wrong, but don't have a mac to test on.

Fixes godotengine#76338.

Blender 3.6 imports fail with:

```
TypeError: Converting py args to operator properties: : keyword "export_nla_strips" unrecognized
```

The `export_nla_strips` flag was removed and replaced with `export_animation_mode`.
In 3.6.0-3.6.21, this option does not exist at all and causes the failure above.
In 3.6.22, this option was re-added, but does nothing.
See https://projects.blender.org/blender/blender-addons/commit/96a73cb664bca687b7ea2e464c4d08f8082d5012.

We now need to check the blender version to determine what flags to use.
This adds an additional shell command before every import.
We might consider caching the version, but we'd have to invalidate the cache if the blender version or path changes.

As an aside, the "group animations" setting in Godot does the opposite of what I'd expect.
When `group_tracks=true`, each animation is exported individually.
When `group_tracks=false`, all animations are exported as a single track.
This seems backwards, but I've kept the 3.6 behavior consistent with 3.5.

From https://docs.blender.org/api/3.6/bpy.ops.export_scene.html:

> ACTIONS Actions – Export actions (actives and on NLA tracks) as separate animations.
> ACTIVE_ACTIONS Active actions merged – All the currently assigned actions become one glTF animation.

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@akien-mga akien-mga merged commit 3cf1767 into godotengine:master Oct 2, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga changed the title Update blender export flags for 3.6. Update Blender export flags for 3.6. Oct 3, 2023
@rcorre rcorre deleted the blend-import-76338 branch October 22, 2023 18:07
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

@Uradamus
Copy link

Uradamus commented Dec 4, 2023

The Blender 4 tickets may have been closed prematurely. Just launched Godot 4.2 and after I dropped the Cyclops add-on into a new project I got a pop up asking me to provide a valid path to a Blender 3 install, which was pre-filled with the path to my Steam install of Blender which updated to 4.0 not long ago. I had to hunt down an old back-up version of Blender 3.x to point it at instead to get the add-on to import (since it comes with a bunch of .blend files). I'm also using Godot from Steam if that matters any.

EDIT: I spoke too soon; the Blender 3 version I pointed at didn't end up working, probably too old now (Blender 3.0). The console was flooded with errors about not being able to open the files. And the import pop-up kept flashing over and over again in an endless loop, had to kill Godot. Was a real mess.

EDIT2: Grabbed the latest 3.6.5 LTS build from the Blender website and replaced the 3.0 install with that and Godot is back to complaining about wanting a valid path with the error: "Unexpected --version output from Blender binary at: /path/to/blender"

@YuriSizov
Copy link
Contributor

@Uradamus As mentioned in the closed proposal, please open an issue in this repository if something is not working correctly. Comments on merged PRs are easy to miss and hard to track.

If you think the proposal itself still has merits and should be reopened, you can leave a comment about it on the proposal itself (closed proposal is "resolved" but the discussion can still continue and it can be reopened).

@Uradamus
Copy link

Uradamus commented Dec 4, 2023

I'm at the point where I will probably end up dropping Godot, I'm so tired of all the troubles with the import pipeline for 3D stuff. So fix it, don't fix it, doesn't matter to me anymore. I've wasted way to much time on this already. Focusing so heavily on GLTF was a mistake, this is just another symptom of that.

The other big area where GLTF has been totally broken has been with shared animation libraries meant to be used by several models. I spent weeks trying to get it working to no avail and now these troubles with getting Blender working with it. I only bring it up in case no one else ran into it, but I'm wiping my hands of this mess for now. Maybe I'll revisit in a year or two to see if anythings been improved or at least better documented if nothing else. I'm just far too fed up to put any more energy into this right now (nothing against you or your suggestions, this is just frustration with all the time wasted with nothing to show for it and continuing to hit walls over stuff that should have been sorted already like this.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure to import .blend with Blender package from Arch Linux
6 participants